fix: mutable default arg in Memory and improve robustness - #1
Conversation
- Fix critical bug: Memory.messages used mutable default [] causing all instances to share the same list. Changed to Field(default_factory=list). - Add max_steps validation in MiniAgent.__init__() to reject values < 1. - Handle empty tool output gracefully in act() to avoid slicing empty string. - Add security warning when BashExecutor executes with shell=True. - Export Role and LLMResponse in __init__.py for full public API access.
There was a problem hiding this comment.
Pull request overview
This PR addresses a shared-state bug in the agent memory model and adds a few small safety/robustness improvements across the agent runtime and public API surface.
Changes:
- Fix
Memory.messagesmutable default by switching toField(default_factory=list). - Add
max_stepsvalidation and improve tool-output previewing for empty outputs. - Warn on
BashExecutorusage ofshell=Trueand export additional public API symbols (Role,LLMResponse) frommini_agent.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| mini_agent/tools.py | Adds a security warning when executing shell commands via shell=True. |
| mini_agent/schema.py | Fixes shared mutable default for Memory.messages using default_factory. |
| mini_agent/agent.py | Validates max_steps and improves logging output for empty tool results. |
| mini_agent/init.py | Exposes Role and LLMResponse in the package public API. |
Comments suppressed due to low confidence (1)
mini_agent/tools.py:156
BashExecutor.executeis declaredasyncbut uses blockingsubprocess.run(...), which will block the event loop and can stall concurrent agent steps. Consider running the subprocess viaasyncio.create_subprocess_shell/execor offloading the blocking call withasyncio.to_threadto keep the async API non-blocking.
result = subprocess.run(
command,
shell=True,
capture_output=True,
text=True,
timeout=30
)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3504e58cb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| warnings.warn( | ||
| "BashExecutor uses shell=True which is a security risk. " | ||
| "Only use with trusted input.", | ||
| stacklevel=2, | ||
| ) |
There was a problem hiding this comment.
Emitting warnings.warn(...) inside BashExecutor.execute can make every bash call fail in environments that run with PYTHONWARNINGS=error or -W error (common in CI/tests): warn raises UserWarning, it is caught by the broad except Exception, and the method returns success=False before/without running the command. This turns a diagnostic warning into a functional regression for those runtimes.
Useful? React with 👍 / 👎.
Summary
This PR fixes a critical bug and adds several robustness improvements:
🐛 Critical Bug Fix
schema.py:Memory.messagesused a mutable default argument (messages: List[Message] = []). In Python, mutable defaults are shared across all instances of a class — meaning multipleMemory()instances would share the same message list, causing cross-contamination between agents. Fixed by usingField(default_factory=list).🔧 Robustness Improvements
agent.py: Addmax_stepsvalidation —MiniAgent(max_steps=0)or negative values now raiseValueErrorimmediately instead of silently producing no output.agent.py: Handle empty tool output gracefully —result_content[:100]on an empty string produced""with trailing..., now shows(empty)instead.🔒 Security
tools.py: Addwarnings.warn()whenBashExecutorexecutes commands withshell=True, so users are aware of the security implications.📦 Public API
__init__.py: ExportRoleandLLMResponseso users can import the full public API frommini_agentwithout reaching into submodules.